-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(observability): ensure internal rate limiting is logged #1151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Closes #841 Signed-off-by: Luke Steensen <[email protected]>
@@ -169,6 +169,7 @@ fn main() { | |||
format!("vector={}", level), | |||
format!("codec={}", level), | |||
format!("file_source={}", level), | |||
format!("tower_limit=trace"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, do we want to use {}
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one trace event in the whole crate and it's at trace
level, so it's the only level that will ever show anything.
I'd actually like for this to be at warn
, but that seemed aggressive to hardcode into a library. If there were a way to translate into our desired levels, then I'd definitely use {}
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this might happen more often this will enable it for every log level. That seems a bit wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative would be that it's only ever logged when we're at trace
everywhere, which I don't think fixes the issue of these being visible during normal operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point, not sure how to solve that then :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have people thought much about appropriate log levels for libraries? We could change tower-limit to warn
here, but I'm not sure that's something everyone would want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hawkw any thoughts?
I'm going to merge for now so we get the functionality, but we can continue the discussion and revisit if we come up with something better. |
Closes #841
Tested locally and works as expected.